Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test(core): merge all package test reports #6261

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

test(core): merge all package test reports #6261

wants to merge 14 commits into from
+75 −5

Conversation

jpinkney-aws
Copy link
Contributor

@jpinkney-aws jpinkney-aws commented Dec 17, 2024 •
edited
Loading

Problem

  • CI test reporting only captures the last package's results
  • Running npm run testE2E only preserves toolkit's report.xml, losing other package results

Solution

  • Generate individual report.xml files per subproject
  • Consolidate all existing package reports into root .test-reports/report.xml
  • This is done in the buildspecs themselves rather than directly in the package.json (the original approach to solve this problem) because mac/linux and windows have two different ways of getting the last error code, resulting in a complicated package.json for something thats only needed in our codebuilds
  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jpinkney-aws
test(core): merge all package test reports
0b2aed9 
Problem:
- CI test reporting only captures the last package's results
- Running `npm run testE2E` only preserves toolkit's report.xml, losing other package results

Solution:
- Generate individual report.xml files per subproject
- Consolidate all existing package reports into root .test-reports/report.xml
@jpinkney-aws
fixup
081c031
@jpinkney-aws
test failures
5ef73cf
@jpinkney-aws
failure
dfe2892
@jpinkney-aws
test
5273b20
@jpinkney-aws
test
5b6e145
@jpinkney-aws
test
adf5ca6
@jpinkney-aws
test
4e5655f
@jpinkney-aws
fixup
bb0bf30
@jpinkney-aws
Copy link
Contributor Author

/runIntegrationTests

@github-actions GitHub Actions
Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@jpinkney-aws
Copy link
Contributor Author

/runIntegrationTests

@Hweinstock
Copy link
Contributor

What are the tradeoffs for merging the report versus having one per extension? Curious because for code coverage we have separate ones.

@jpinkney-aws
Copy link
Contributor Author

If you don't merge them you can't see them in the ui here: https://d1ihu6zq92vp9p.cloudfront.net/487310e6-ebfc-4fef-a0b1-b441147b3c23/report.html. That's really the issue i'm trying to solve. It's pretty annoying when an amazon q test fails and then you have to manually look through the logs to find the problem

I don't think that restriction applies to codecov since you can upload multiple different reports at the same time

@jpinkney-aws
Copy link
Contributor Author

/runIntegrationTests

@jpinkney-aws
Copy link
Contributor Author

/runIntegrationTests

justinmk3
justinmk3 reviewed Dec 17, 2024
View reviewed changes
buildspec/linuxE2ETests.yml
@@ -37,7 +37,7 @@ phases:
commands:
- export HOME=/home/codebuild-user
# Ignore failure until throttling issues are fixed.
- xvfb-run npm run testE2E
- xvfb-run npm run testE2E; PREVIOUS_TEST_EXIT_CODE=$? npm run mergeReports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can avoid this env var by passing the exit code directly as an arg

Suggested change
- xvfb-run npm run testE2E; PREVIOUS_TEST_EXIT_CODE=$? npm run mergeReports
- xvfb-run npm run testE2E; npm run mergeReports -- "$?"

package.ts shows how to get script args:

aws-toolkit-vscode/scripts/package.ts

Lines 40 to 42 in 5b5df85

const givenArgs = process.argv.slice(2)
const validOptions = ['--debug', '--no-clean', '--feature']
const expectValue = ['--feature']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmk3 justinmk3 justinmk3 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jpinkney-aws @Hweinstock @justinmk3